FINERACT-2081: async liquibase #4178
Conversation
| @@ -131,6 +131,8 @@ fineract.events.external.producer.kafka.admin.extra-properties=${FINERACT_EXTERN | |||
|
|
|||
| fineract.task-executor.default-task-executor-core-pool-size=${FINERACT_DEFAULT_TASK_EXECUTOR_CORE_POOL_SIZE:10} | |||
| fineract.task-executor.default-task-executor-max-pool-size=${FINERACT_DEFAULT_TASK_EXECUTOR_MAX_POOL_SIZE:100} | |||
| fineract.task-executor.tenant-upgrade-task-executor-core-pool-size=${FINERACT_TENANT_UPGRADE_TASK_EXECUTOR_CORE_POOL_SIZE:1} | |||
| fineract.task-executor.tenant-upgrade-task-executor-max-pool-size=${FINERACT_TENANT_UPGRADE_TASK_EXECUTOR_MAX_POOL_SIZE:10} | |||
There was a problem hiding this comment.
I think let's have this as 1 by default to keep backward compatibility. I can imagine issues where for establishing the tenant connections on 10 threads, each eating up let's say 20-30 connections (because of the initial tenant connection pool size), we'd have 200-300 connections alive at the same time which for a smaller deployment could be an issue.
There was a problem hiding this comment.
I agree with you on backward compatibility, but a 10 tenant deployment being a small one is a bit of a stretch 😄
|
|
||
| private final FineractProperties fineractProperties; | ||
|
|
||
| @Bean("tenantUpgradeThreadPoolTaskExecutor") |
There was a problem hiding this comment.
Didn't you introduce a constant class for these pool bean name constants?
There was a problem hiding this comment.
I did, but i made this one parallel to the one i introduced it in (towards the last updates on the other pr), i forgot to update this one aswell. (Since i inlined it it doesn't matter tho.)
| @Bean("tenantUpgradeThreadPoolTaskExecutor") | ||
| public ThreadPoolTaskExecutor tenantUpgradeThreadPoolTaskExecutor() { | ||
| ThreadPoolTaskExecutor threadPoolTaskExecutor = new ThreadPoolTaskExecutor(); | ||
| threadPoolTaskExecutor.setCorePoolSize(fineractProperties.getTaskExecutor().getTenantUpgradeTaskExecutorCorePoolSize()); |
There was a problem hiding this comment.
Let's set the queue size as well (I'm not sure whats the default).
There was a problem hiding this comment.
Default is Integer.MAX_VALUE, so it really should be set. Added config for it.
| ThreadPoolTaskExecutor threadPoolTaskExecutor = new ThreadPoolTaskExecutor(); | ||
| threadPoolTaskExecutor.setCorePoolSize(fineractProperties.getTaskExecutor().getTenantUpgradeTaskExecutorCorePoolSize()); | ||
| threadPoolTaskExecutor.setMaxPoolSize(fineractProperties.getTaskExecutor().getTenantUpgradeTaskExecutorMaxPoolSize()); | ||
| return threadPoolTaskExecutor; |
There was a problem hiding this comment.
I believe you called initialize() last time on the threadpool, why missing here or am I just wrong?
There was a problem hiding this comment.
Hmm seems like spring calls it when instantiating a TaskExecutor. Inlined it so added the init call.
| ThreadPoolTaskExecutor threadPoolTaskExecutor = new ThreadPoolTaskExecutor(); | ||
| threadPoolTaskExecutor.setCorePoolSize(fineractProperties.getTaskExecutor().getTenantUpgradeTaskExecutorCorePoolSize()); | ||
| threadPoolTaskExecutor.setMaxPoolSize(fineractProperties.getTaskExecutor().getTenantUpgradeTaskExecutorMaxPoolSize()); | ||
| return threadPoolTaskExecutor; |
There was a problem hiding this comment.
Don't we need setWaitForTasksToCompleteOnShutdown set?
There was a problem hiding this comment.
upgradeIndividualTenants() waits for the tasks to complete (future.get()), so not needed here
|
|
||
| private final FineractProperties fineractProperties; | ||
|
|
||
| @Bean("tenantUpgradeThreadPoolTaskExecutor") |
There was a problem hiding this comment.
Btw this could absolutely be an inline thread pool task executor instead of a bean since it's single use. That'll prevent any misusage through injecting the bean somewhere mistakenly when the pool is already shut down.
There was a problem hiding this comment.
Good idea, inlined it.
magyari-adam
force-pushed
the
feature/FINERACT-2081-liqui
branch
2 times, most recently
from
4b1b733
to
b8aafc9
Compare
magyari-adam
force-pushed
the
feature/FINERACT-2081-liqui
branch
from
b8aafc9
to
1bdeb5e
Compare
Description
Makes liquibase tenant upgrades async.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.